Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MLIR][OpenMP] Keep -verify-openmp-ops output as dependency #99638

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

skatrak
Copy link
Contributor

@skatrak skatrak commented Jul 19, 2024

Currently, the mlir-tblgen -verify-openmp-ops pseudo-backend, which only performs an OpenMP dialect-specific set of checks and produces no output, is prevented from being added as a dependency to the MLIROpenMPOpsIncGen tablegen target.

However, a consequence of this is that it is not triggered with every modification of the OpenMPOps.td file it's intended to check, although it should. This patch fixes the issue by letting the empty output file to be added to the TABLEGEN_OUTPUT CMake variable used by the add_public_tablegen_target command below to set up dependencies.

Currently, the `mlir-tblgen -verify-openmp-ops` pseudo-backend which only
performs an OpenMP dialect-specific set of checks and produces no output is
prevented from being added as a dependency to the `MLIROpenMPOpsIncGen`
tablegen target.

However, a consequence of this is that it is not triggered with every
modification of the OpenMPOps.td file it's intended to check, as it should.
This patch fixes this issue by letting the empty output file being added to the
`TABLEGEN_OUTPUT` CMake variable used by the `add_public_tablegen_target`
command below to set up dependencies.
@llvmbot
Copy link
Member

llvmbot commented Jul 19, 2024

@llvm/pr-subscribers-mlir

Author: Sergio Afonso (skatrak)

Changes

Currently, the mlir-tblgen -verify-openmp-ops pseudo-backend, which only performs an OpenMP dialect-specific set of checks and produces no output, is prevented from being added as a dependency to the MLIROpenMPOpsIncGen tablegen target.

However, a consequence of this is that it is not triggered with every modification of the OpenMPOps.td file it's intended to check, although it should. This patch fixes the issue by letting the empty output file to be added to the TABLEGEN_OUTPUT CMake variable used by the add_public_tablegen_target command below to set up dependencies.


Full diff: https://github.com/llvm/llvm-project/pull/99638.diff

1 Files Affected:

  • (modified) mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt (+7-7)
diff --git a/mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt b/mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt
index d3422f6e48b06..dd349d1392e7b 100644
--- a/mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt
+++ b/mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt
@@ -4,14 +4,14 @@ add_public_tablegen_target(omp_common_td)
 
 set(LLVM_TARGET_DEFINITIONS OpenMPOps.td)
 
-# Run the OpenMP verifier tablegen pseudo-backend while preventing the produced
-# dummy output from being added as a dependency to any tablegen targets defined
-# below.
-set(TABLEGEN_OUTPUT_TMP ${TABLEGEN_OUTPUT})
+# The OpenMP verifier tablegen pseudo-backend does not produce any output, but
+# mlir_tablegen expects an output file name to be passed. An empty "no-output"
+# file is created by the statement below.
+#
+# This output will be added to the list of dependencies of the
+# MLIROpenMPOpsIncGen target below, which results in triggering this
+# verification pass every time OpenMPOps.td is modified and recompiled.
 mlir_tablegen(no-output -verify-openmp-ops)
-file(REMOVE ${CMAKE_CURRENT_BINARY_DIR}/no-output ${CMAKE_CURRENT_BINARY_DIR}/no-output.d)
-set(TABLEGEN_OUTPUT ${TABLEGEN_OUTPUT_TMP})
-unset(TABLEGEN_OUTPUT_TMP)
 
 mlir_tablegen(OpenMPOpsDialect.h.inc -gen-dialect-decls -dialect=omp)
 mlir_tablegen(OpenMPOpsDialect.cpp.inc -gen-dialect-defs -dialect=omp)

@llvmbot
Copy link
Member

llvmbot commented Jul 19, 2024

@llvm/pr-subscribers-mlir-openmp

Author: Sergio Afonso (skatrak)

Changes

Currently, the mlir-tblgen -verify-openmp-ops pseudo-backend, which only performs an OpenMP dialect-specific set of checks and produces no output, is prevented from being added as a dependency to the MLIROpenMPOpsIncGen tablegen target.

However, a consequence of this is that it is not triggered with every modification of the OpenMPOps.td file it's intended to check, although it should. This patch fixes the issue by letting the empty output file to be added to the TABLEGEN_OUTPUT CMake variable used by the add_public_tablegen_target command below to set up dependencies.


Full diff: https://github.com/llvm/llvm-project/pull/99638.diff

1 Files Affected:

  • (modified) mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt (+7-7)
diff --git a/mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt b/mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt
index d3422f6e48b06..dd349d1392e7b 100644
--- a/mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt
+++ b/mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt
@@ -4,14 +4,14 @@ add_public_tablegen_target(omp_common_td)
 
 set(LLVM_TARGET_DEFINITIONS OpenMPOps.td)
 
-# Run the OpenMP verifier tablegen pseudo-backend while preventing the produced
-# dummy output from being added as a dependency to any tablegen targets defined
-# below.
-set(TABLEGEN_OUTPUT_TMP ${TABLEGEN_OUTPUT})
+# The OpenMP verifier tablegen pseudo-backend does not produce any output, but
+# mlir_tablegen expects an output file name to be passed. An empty "no-output"
+# file is created by the statement below.
+#
+# This output will be added to the list of dependencies of the
+# MLIROpenMPOpsIncGen target below, which results in triggering this
+# verification pass every time OpenMPOps.td is modified and recompiled.
 mlir_tablegen(no-output -verify-openmp-ops)
-file(REMOVE ${CMAKE_CURRENT_BINARY_DIR}/no-output ${CMAKE_CURRENT_BINARY_DIR}/no-output.d)
-set(TABLEGEN_OUTPUT ${TABLEGEN_OUTPUT_TMP})
-unset(TABLEGEN_OUTPUT_TMP)
 
 mlir_tablegen(OpenMPOpsDialect.h.inc -gen-dialect-decls -dialect=omp)
 mlir_tablegen(OpenMPOpsDialect.cpp.inc -gen-dialect-defs -dialect=omp)

Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@skatrak skatrak merged commit cba63c0 into llvm:main Jul 24, 2024
10 checks passed
@skatrak skatrak deleted the omp-tblgen-verify-no-skip branch July 24, 2024 10:38
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
Currently, the `mlir-tblgen -verify-openmp-ops` pseudo-backend, which
only performs an OpenMP dialect-specific set of checks and produces no
output, is prevented from being added as a dependency to the
`MLIROpenMPOpsIncGen` tablegen target.

However, a consequence of this is that it is not triggered with every
modification of the OpenMPOps.td file it's intended to check, although
it should. This patch fixes the issue by letting the empty output file
to be added to the `TABLEGEN_OUTPUT` CMake variable used by the
`add_public_tablegen_target` command below to set up dependencies.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250680
Harini0924 pushed a commit to Harini0924/llvm-project that referenced this pull request Aug 1, 2024
Currently, the `mlir-tblgen -verify-openmp-ops` pseudo-backend, which
only performs an OpenMP dialect-specific set of checks and produces no
output, is prevented from being added as a dependency to the
`MLIROpenMPOpsIncGen` tablegen target.

However, a consequence of this is that it is not triggered with every
modification of the OpenMPOps.td file it's intended to check, although
it should. This patch fixes the issue by letting the empty output file
to be added to the `TABLEGEN_OUTPUT` CMake variable used by the
`add_public_tablegen_target` command below to set up dependencies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants